Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MM-19315] Replace old Logger with mlog.Logger (mattermost package) #130

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

willypuzzle
Copy link

@willypuzzle willypuzzle commented Dec 3, 2024

Summary

This PR replace the old Logger with the more modern mlog.Logger.

Ticket Link

mattermost/mattermost#14407

Release note:

Added LogFormat option, possible values are plain or json (default value: plain)

Refactored the logging system in both Android and Apple notification servers. Replaced `logger.Infof` with `logger.Info` for a more structured logging approach, using `mlog.String`, `mlog.Int`, and `mlog.Err` to provide additional context. This change enhances readability and allows for better log analysis.
The logging statements in the server and notification files have been refactored to improve readability. The changes include replacing `logger.Errorf` with `logger.Error` and using structured logging fields instead of string formatting. This makes it easier to understand the log messages and their associated data.
Switched from using Panic to Fatal in the server's error handling mechanism. This change ensures that any serious errors will cause the program to exit immediately, rather than just panicking and potentially leaving the server in an unstable state.
The logger type has been updated from *Logger to *mlog.Logger in the AndroidNotificationServer, AppleNotificationServer, and Server structures. This change also affects the NewAndroidNotificationServer, NewAppleNotificationServer functions where the logger parameter type is updated accordingly.
Refactored the ConfigPushProxy struct. Deprecated LogFileLocation, EnableConsoleLog, and EnableFileLog for backward compatibility. Introduced LoggingCfgFile and LoggingCfgJSON as replacements. No changes to other existing fields.
Removed the automatic enabling of console log when both console and file logs are disabled. Also, removed the creation and handling of log files within the configuration loading function. This simplifies the function by focusing it solely on loading configurations.
The main function has been updated to include a more robust logger initialization process. This includes error handling for the creation of a new logger, as well as its configuration. If no logging is defined, a default config (console output) is used. A separate function has been added to provide this default logging configuration. The server now also ensures that the logger is properly shut down when it's no longer needed.
Upgraded the Go version from 1.21 to 1.22 and updated various dependencies to their latest versions. This includes updates to firebase, gorilla handlers, mux, prometheus client_golang, common, golang.org/x/net and oauth2 among others. Also added new dependencies such as github.com/mattermost/mattermost/server/public and github.com/francoispqt/gojay.
@mattermost-build
Copy link
Contributor

Hello @willypuzzle,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@enahum enahum requested review from agnivade and larkox December 3, 2024 18:38
@enahum enahum added the 1: Dev Review Requires review by a core commiter label Dec 3, 2024
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just a couple of comments to discuss.

server/config_push_proxy.go Outdated Show resolved Hide resolved
server/config_push_proxy.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
The default logging configuration has been refactored to support both console and file logging. The logic for generating the default config has been moved from main.go to a new file, server/logging.go. This change allows the application to dynamically choose between console or file logging based on the 'EnableFileLog' flag in the configuration.
@agnivade
Copy link
Member

agnivade commented Dec 4, 2024

Thank you @willypuzzle. I will be taking a look at this soon.

Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more comments about tests.

main.go Outdated Show resolved Hide resolved
server/logging.go Outdated Show resolved Hide resolved
Upgraded the Go version from 1.21 to 1.22 and updated various dependencies to their latest versions. Also, added new dependencies required for the project's functionality.
The Docker images for Go and Golint have been updated to newer versions. The new version of the Go image includes various performance improvements and bug fixes. Similarly, the Golint image has also been updated to include the latest linting rules and enhancements.
The logger initialization in various test files has been updated. Instead of using the NewLogger function, we are now using mlog.NewLogger(). This change affects android_notification_test.go, metrics_test.go, and server_test.go.
- Added error checks after initializing new loggers in various test functions to ensure no silent failures.
- Simplified the condition check for enabling file logs by directly using the boolean value.
Introduced a new set of unit tests for the server's logging functionality. These tests cover various aspects such as escaping double quotes, default console log configuration, file log configuration and overall logging configuration. Also added a helper function to generate random strings for testing purposes.
The logging settings have been simplified and restructured. The previous console and file log enablement flags have been removed. Instead, a new approach has been introduced where the logging configuration is now handled through a dedicated config file or directly via JSON.
A new JSON configuration file for logging has been added. This includes settings for console type output, plain formatting, and color enabling. It also specifies minimum lengths for level and message, as well as the inclusion of caller information. Different log levels from debug to panic have been defined with corresponding IDs and optional color or stacktrace attributes.
The logger initialization and configuration process has been refactored for better code organization. The logic previously in the main function is now encapsulated within a new function, NewMlogLogger, in the server package. This change also includes updates to logging tests to accommodate the new structure.
This update includes several significant changes:
- The release is compatible with all versions of Mattermost server and mobile.
- Notably, Google Cloud Messaging (GCM) service has been deprecated as of April 10, 2018, and will be removed by April 11, 2019. Migration to Firebase Cloud Messaging (FCM) is necessary.
- The old Logger system has been replaced with a new one.
- Deprecated types EnableConsoleLog, EnableFileLog, and LogFileLocation from the config have been replaced.
- Lastly, we've switched from go version 1.21 to go version 1.22 for improved performance and stability.
The README file has been updated to include a reference to the CHANGELOG. This will provide users with more detailed information about the changes made in each release.
@willypuzzle
Copy link
Author

@larkox @agnivade I made all the requested changes and improvements.

Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Just two minor comments.

server/logging.go Outdated Show resolved Hide resolved
server/logging_test.go Outdated Show resolved Hide resolved
@willypuzzle
Copy link
Author

@agnivade please, would you review this PR?

@agnivade
Copy link
Member

Sorry for the delay! Yes I will take a look.

Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience @willypuzzle!

server/logging.go Outdated Show resolved Hide resolved
server/config_push_proxy.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
config/logging-cfg-file.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
config/mattermost-push-proxy.sample.json Outdated Show resolved Hide resolved
The logging configuration has been refactored to simplify the process. Deprecated fields have been removed from the ConfigPushProxy struct and replaced with a single LogFormat field. The NewMlogLogger function now uses this new field to determine the log format, defaulting to "plain" if an invalid value is provided. Additionally, helper functions have been created to build the logger configuration for both console and file outputs based on these changes. Corresponding tests have also been updated accordingly.
The logging configuration file has been removed. Logging settings have been directly integrated into the main configuration file. This includes disabling console and file logs by default, specifying log format, and removing specific log level definitions.
The configuration has been updated to enable console logging. This change will allow for better tracking and debugging of the application's activities.
The version of golangci-lint used in the Docker image has been downgraded. This was done to ensure compatibility with other components and improve overall stability.
@agnivade agnivade self-requested a review December 11, 2024 14:15
@agnivade
Copy link
Member

Thanks @willypuzzle. I will take another look soon.

@willypuzzle
Copy link
Author

@agnivade There is a job not related to my PR that is blocking the flow. Please, would you relaunch it?

@willypuzzle
Copy link
Author

@agnivade please, could you act on this PR? Thanks!

@agnivade
Copy link
Member

Apologies for the delay here @willypuzzle. I will be on vacation next week. I will be able to take a look at this once I am back.

Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good @willypuzzle. And thank you for your patience with this. It's a big change, so we need to be thorough here. I've left some more comments to take care of.

server/android_notification_server.go Outdated Show resolved Hide resolved
server/logging.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
server/android_notification_server.go Outdated Show resolved Hide resolved
server/android_notification_test.go Outdated Show resolved Hide resolved
server/config_push_proxy.go Outdated Show resolved Hide resolved
server/config_push_proxy.go Outdated Show resolved Hide resolved
server/metrics_test.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server_test.go Outdated Show resolved Hide resolved
willypuzzle and others added 7 commits December 23, 2024 12:17
The logging functionality and its associated tests have been removed from the server. This includes the deletion of functions for creating new loggers, building log configurations, and configuring console and file logs. The changes aim to simplify the codebase by eliminating unnecessary components.
The import statements for the 'mlog' package and a few others have been restructured across several server files. This change improves readability and consistency of the codebase.
The existing logging system has been replaced with a more flexible one. The new logger is based on the mlog library, which allows for different log formats and configurable targets. This change also includes updates to the logger configuration process and removes some helper methods that were previously used in the application. Tests have been updated accordingly to reflect these changes.
The logging messages in the Android and Apple notification servers have been updated to improve clarity. The changes include:
- Removed specific identifiers from the log messages when sending push notifications.
- Standardized the naming convention of 'ack_id' across all logs.
- Simplified log message when initializing apple notification server with AuthKey.
- Updated acknowledgment delivery receipt log message for better readability.
Added logic to ensure at least one type of logging (console or file) is enabled. If both are disabled, console logging will be turned on by default. For file logging, if the log file location is not specified, it will create a new directory for logs and set the log file location there. If the directory creation fails, it logs in the current directory itself. Also ensured that if the log file does not exist, it gets created. Additionally, refined timeout settings by setting defaults if they're not provided in the configuration.
Simplified the logger initialization process by removing unnecessary comments and renaming the function used to create a new logger. This change makes the code cleaner and easier to read.
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this @willypuzzle ! There were some more changes needed. But I went ahead and did those myself since you have already spent so much time on this.

@agnivade
Copy link
Member

/update-branch

@mattermost-build
Copy link
Contributor

Error trying to update the PR.
Please do it manually.

@agnivade
Copy link
Member

@toninis - Any idea why CI is not running for this PR?

@agnivade agnivade added 2: Reviews Complete All reviewers have approved the pull request Do Not Merge Should not be merged until this label is removed and removed 1: Dev Review Requires review by a core commiter labels Dec 23, 2024
@willypuzzle
Copy link
Author

Thank you for this @willypuzzle ! There were some more changes needed. But I went ahead and did those myself since you have already spent so much time on this.

@agnivade no prob! ask the same, I'm happy to have practice with go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Reviews Complete All reviewers have approved the pull request Contributor Do Not Merge Should not be merged until this label is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants